New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(feat) Add option to fetch Firefox Nightly #5467
Conversation
Add Firefox support to BrowserFetcher and the install script. By default, the latest Firefox Nightly is downloaded directly from archive.mozilla.org (dmg, tar.bz2 and zip) This also required changes that impact `puppeteer.launch()` and `puppeteer.executablePath()` Fixes puppeteer#5151
@@ -37,13 +37,13 @@ npm i puppeteer | |||
# or "yarn add puppeteer" | |||
``` | |||
|
|||
Note: When you install Puppeteer, it downloads a recent version of Chromium (~170MB Mac, ~282MB Linux, ~280MB Win) that is guaranteed to work with the API. To skip the download, see [Environment variables](https://github.com/puppeteer/puppeteer/blob/v2.1.1/docs/api.md#environment-variables). | |||
Note: When you install Puppeteer, it downloads a recent version of Chromium (~170MB Mac, ~282MB Linux, ~280MB Win) that is guaranteed to work with the API. To skip the download, or to download a different browser, see [Environment variables](https://github.com/puppeteer/puppeteer/blob/v2.1.1/docs/api.md#environment-variables). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, we probably don’t want to do this right now, but in the future we could work towards npm install puppeteer
fetching both Chromium and Firefox binaries by default. I like your approach of taking one step at a time 👍🏻
I’ll take a closer look during the week, but just wanted to drop a note saying I’m super excited about this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, just some questions and suggestions. @hanselfmu could you PTAL as well, especially w.r.t. dropping Node.js v8?
}).on('error', e => reject(e)); | ||
|
||
function parseVersion() { | ||
const regex = /firefox\-(?<version>\d\d)\..*/gm; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loving the use of named capture groups here!
This will fail on Node.js v8.x.x so we'd want to land ##5365 first. @hanselfmu could you please take another look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR to drop Node.js v8.x support is labeled as a breaking change, which means landing it will be in Puppeteer v3. I think we have two options now:
- land both of them in v3;
- temporarily use Node.js-8-compatible code for now, and when v3 lands, update the legacy code.
@mjzffr what do you think? Do you have other ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO major version bumps are no big deal. We shouldn't hold back on publishing v3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'll publish v3 now and include this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hanselfmu @mathiasbynens What happened with this? This PR doesn't seem to be available in a release version yet.
Co-Authored-By: Mathias Bynens <mathias@qiwi.be>
Co-Authored-By: Mathias Bynens <mathias@qiwi.be>
Co-Authored-By: Mathias Bynens <mathias@qiwi.be>
I'm fine with swapping in node-8-compatible code if you want to delay v3.
Based on Mathias' reply, I'm assuming you'll move forward with v3 soon, but
if that's not the case, just ping me and I'll update the PR. I think I've
addressed all the other review feedback.
…On Mon, Mar 9, 2020 at 9:31 AM Mathias Bynens ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In install.js
<#5467 (comment)>:
> - width: 20,
- total: totalBytes,
+ function getFirefoxNightlyVersion(host) {
+ const https = require('https');
+ const promise = new Promise((resolve, reject) => {
+ let data = '';
+ logPolitely(`Requesting latest Firefox Nightly version from ${host}`);
+ https.get(host + '/', r => {
+ r.on('data', chunk => {
+ data += chunk;
+ });
+ r.on('end', parseVersion);
+ }).on('error', e => reject(e));
+
+ function parseVersion() {
+ const regex = /firefox\-(?<version>\d\d)\..*/gm;
IMHO major version bumps are no big deal. We shouldn't hold back on
publishing v3.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5467?email_source=notifications&email_token=ABN6ZBTKZONSB5ZAB74LTLDRGTVS3A5CNFSM4K54YPJKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCYP5CDI#discussion_r389666663>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABN6ZBSNBRUQTWNNG2HCLMTRGTVS3ANCNFSM4K54YPJA>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Add Firefox support to BrowserFetcher and the install script.
By default, the latest Firefox Nightly is downloaded
directly from archive.mozilla.org (dmg, tar.bz2 and zip)
This also required changes that impact
puppeteer.launch()
and
puppeteer.executablePath()
Fixes #5151